-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proxy] Remove unnecessary Pulsar Client usage from Pulsar Proxy #13836
[Proxy] Remove unnecessary Pulsar Client usage from Pulsar Proxy #13836
Conversation
@lhotari:Thanks for your contribution. For this PR, do we need to update docs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@lhotari:Thanks for providing doc info! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great move!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…che#13836) (cherry picked from commit 324aa1b)
) (cherry picked from commit 324aa1b)
…che#13836) (cherry picked from commit 324aa1b) (cherry picked from commit 25e6b65) (cherry picked from commit 94fd1f5)
…che#13836) (cherry picked from commit 324aa1b) (cherry picked from commit 25e6b65)
Motivation
Pulsar Proxy will create a new Pulsar Client instance for every proxied connection.
Currently 2 threads will be created for every Pulsar Client instance for the internal/IO and external/listener executors. This causes a lot of overhead and inefficiency in the Pulsar Proxy.
It turns out that the PulsarClientImpl instance is unnecessary in Pulsar Proxy. The Proxy code doesn't need the instance at all and therefore it can be removed.
While working on the changes, it was noticed that the ConnectionPool cleanup code is wrong. It is closing the EventLoopGroup provided to the ConnectionPool although ConnectionPool doesn't create this resource. In addition, closing didn't close existing connections.
Additional context
This PR replaces PR #13834
Modifications